fix(segment-button): update checked state on render by sean-perkins · Pull Request #26970 · ionic-team/ionic-framework

sean-perkins · 2023-03-15T15:25:44Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
- Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
Build (npm run build) was run locally and any changes were pushed
Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

In v7, asynchronously assigning a value to ion-segment and ion-segment-button can cause the ion-segment to render without an active value (segment button does not show as checked).

Issue URL: resolves #26830

What is the new behavior?

The ion-segment-button checked state is synced with the parent ion-segment, when the component finishes the initial render.

Does this introduce a breaking change?

Yes
No

Other information

Dev-build: 7.0.0-dev.11678736268.18a0beb1 ✅

bolt-new-by-stackblitz · 2023-03-15T15:25:48Z

Run & review this pull request in StackBlitz Codeflow.

lincolnthree · 2023-03-28T17:22:23Z

Does this need to be tagged for review? What can I do to help this along?

sean-perkins · 2023-03-28T18:46:20Z

Opening this up for review. I identified that calling updateState() resolves the problematic behavior in the reproduction (as well as Lincoln has done the same).

I left this in draft for a bit because I wanted to see if there is an easy way to write tests around this (the reproduction requires a bit of rxjs + Angular). I haven't had the time to do so yet.

I think the risk of this change is incredibly low (at worst, it is calling updateState() when the state is already set correctly). I'd feel comfortable creating a tech debt ticket to track adding test coverage to this in a follow-up sprint, in order to get this into the next RC.

liamdebeasi · 2023-03-28T18:49:17Z

    };
  }

+  componentDidLoad() {


Threading for discussion:

Are we confident this is a v7 regression? When I triaged this in #26830 (comment), I was able to reproduce this in v6, but perhaps I reproduced a different issue.

Alternatively: Is this a bug that's always been there but is now causing issues because of v7 changes? (possibly the initializeNextTick change?)

Ok I did a bit more digging and this is (fortunately) not an Ionic v7 regression. However, it is much more noticeable now that the initializeNextTick config was removed in Ionic v7.

The problem here is that if the value property on ion-segment-button is set asynchronously, the updateState method is not called again.

So given the following:

<ion-segment value="general"> <ion-segment-button>General</ion-segment-button> </ion-segment>

If you were to asynchronously set the value of the segment button to "general", the segment button would not appear checked.

Below is a test you can use to verify the fix.

segment/test/async/segment.e2e.ts

import { expect } from '@playwright/test'; import { test } from '@utils/test/playwright'; test.describe('segment: async', () => { test('should set checked state when value is set asynchronously', async ({ page, skip }) => { skip.rtl(); skip.mode('md'); await page.setContent(` <ion-segment value="first"> <ion-segment-button>First</ion-segment-button> </ion-segment> `); const segmentButton = page.locator('ion-segment-button'); await segmentButton.evaluate((el: HTMLIonSegmentButtonElement) => el.value = 'first'); await page.waitForChanges(); await expect(segmentButton).toHaveClass(/segment-button-checked/); }); });

If possible, we should merge this into main so v6 devs can benefit from this fix too.

lincolnthree · 2023-03-28T22:27:34Z

Thanks so much guys! Really happy you were able to figure out the root cause of what's happening.

lincolnthree · 2023-04-06T17:41:02Z

Just looping back on this. Anything I can do to help? I noticed it didn't make it into v7, and the issue is occurring quite consistently for my application now. Sorry to be a pest! But I've seen a few releases out and this seems so close to the finish line.

liamdebeasi · 2023-04-06T17:48:18Z

We pulled this into the next sprint to take another look at.

liamdebeasi · 2023-04-10T13:50:52Z

@sean-perkins I added the correct fix as well as a test. Mind taking a look when you get a chance? (I changed from an E2E to a unit test too)

…-checked

sean-perkins · 2023-04-12T01:35:08Z

Looks good to me, I resolved the merge conflict with main. I can't review my own PRs, but I'll mark this as merge when ready.

We can cherry pick the commit into the v6 release branch.

Please check if your PR fulfills the following requirements: - [ ] Tests for the changes have been added (for bug fixes / features) - [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features) - Some docs updates need to be made in the `ionic-docs` repo, in a separate PR. See the [contributing guide](https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation) for details. - [x] Build (`npm run build`) was run locally and any changes were pushed - [x] Lint (`npm run lint`) has passed locally and any fixes were made for failures   Please check the type of change your PR introduces: - [x] Bugfix - [ ] Feature - [ ] Code style update (formatting, renaming) - [ ] Refactoring (no functional changes, no api changes) - [ ] Build related changes - [ ] Documentation content changes - [ ] Other (please describe):  In v7, asynchronously assigning a value to `ion-segment` and `ion-segment-button` can cause the `ion-segment` to render without an active value (segment button does not show as checked).  Issue URL: resolves #26830  - The `ion-segment-button` checked state is synced with the parent `ion-segment`, when the component finishes the initial render. - [ ] Yes - [x] No   Dev-build: `7.0.0-dev.11678736268.18a0beb1` ✅ --------- Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>

This backports #26970 to v6. resolves #26830 Co-authored-by: Sean Perkins <sean@ionic.io>

## Pull request checklist Please check if your PR fulfills the following requirements: - [ ] Tests for the changes have been added (for bug fixes / features) - [ ] Docs have been reviewed and added / updated if needed (for bug fixes / features) - Some docs updates need to be made in the `ionic-docs` repo, in a separate PR. See the [contributing guide](https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation) for details. - [x] Build (`npm run build`) was run locally and any changes were pushed - [x] Lint (`npm run lint`) has passed locally and any fixes were made for failures ## Pull request type   Please check the type of change your PR introduces: - [x] Bugfix - [ ] Feature - [ ] Code style update (formatting, renaming) - [ ] Refactoring (no functional changes, no api changes) - [ ] Build related changes - [ ] Documentation content changes - [ ] Other (please describe): ## What is the current behavior?  In v7, asynchronously assigning a value to `ion-segment` and `ion-segment-button` can cause the `ion-segment` to render without an active value (segment button does not show as checked).  Issue URL: resolves #26830 ## What is the new behavior?  - The `ion-segment-button` checked state is synced with the parent `ion-segment`, when the component finishes the initial render. ## Does this introduce a breaking change? - [ ] Yes - [x] No  ## Other information  Dev-build: `7.0.0-dev.11678736268.18a0beb1` ✅ --------- Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>

sean-perkins added 2 commits March 13, 2023 15:37

fix(segment-button): assign checked state on render

8a0beb1

chore: remove redundant check

48e9f12

Merge branch 'feature-7.0' into fix/v7-segment-button-checked

1ecc581

github-actions Bot added the package: core @ionic/core package label Mar 15, 2023

sean-perkins added 2 commits March 20, 2023 11:07

Merge branch 'feature-7.0' into fix/v7-segment-button-checked

00cbd5a

Merge branch 'feature-7.0' into fix/v7-segment-button-checked

5070b37

Merge branch 'feature-7.0' into fix/v7-segment-button-checked

78a9fc5

sean-perkins marked this pull request as ready for review March 28, 2023 18:46

sean-perkins requested a review from a team as a code owner March 28, 2023 18:46

liamdebeasi reviewed Mar 28, 2023

View reviewed changes

Base automatically changed from feature-7.0 to main March 30, 2023 16:16

Merge branch 'main' into fix/v7-segment-button-checked

684bfe7

liamdebeasi added 4 commits April 10, 2023 13:43

test(segment): add failing test

cf562d0

fix(segment-button): update state on value change

df45f1e

refactor(segment): use spec instead of e2e

23de0b7

add additional check

c8358b3

liamdebeasi and others added 2 commits April 10, 2023 10:10

Merge remote-tracking branch 'origin/main' into fix/v7-segment-button…

f16df08

…-checked

Merge remote-tracking branch 'origin/main' into fix/v7-segment-button…

df4fdcc

…-checked

sean-perkins enabled auto-merge April 12, 2023 01:35

baffioso mentioned this pull request Apr 12, 2023

Add event types to filter toolbar on map and timetable page blockparty-team/blockparty#197

Merged

liamdebeasi approved these changes Apr 12, 2023

View reviewed changes

sean-perkins added this pull request to the merge queue Apr 12, 2023

liamdebeasi removed this pull request from the merge queue due to a manual request Apr 12, 2023

liamdebeasi added this pull request to the merge queue Apr 12, 2023

Merged via the queue into main with commit 16aa977 Apr 12, 2023

liamdebeasi deleted the fix/v7-segment-button-checked branch April 12, 2023 13:41

liamdebeasi mentioned this pull request Apr 12, 2023

fix(segment-button): update checked state on render #27183

Merged

liamdebeasi added a commit that referenced this pull request Apr 12, 2023

fix(segment-button): update checked state on render (#27183)

ae9689b

This backports #26970 to v6. resolves #26830 Co-authored-by: Sean Perkins <sean@ionic.io>

Conversation

sean-perkins commented Mar 15, 2023 • edited by liamdebeasi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Uh oh!

bolt-new-by-stackblitz Bot commented Mar 15, 2023

Uh oh!

lincolnthree commented Mar 28, 2023

Uh oh!

sean-perkins commented Mar 28, 2023

Uh oh!

liamdebeasi Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

liamdebeasi Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

liamdebeasi Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

liamdebeasi Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lincolnthree commented Mar 28, 2023

Uh oh!

lincolnthree commented Apr 6, 2023

Uh oh!

liamdebeasi commented Apr 6, 2023

Uh oh!

liamdebeasi commented Apr 10, 2023

Uh oh!

sean-perkins commented Apr 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sean-perkins commented Mar 15, 2023 •

edited by liamdebeasi

Loading